Update callers of isTriviallyReMaterializable to check trivialness #160319
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a preparatory change for an upcoming reorganization of our rematerialization APIs. Despite the interface being documented as "trivial" (meaning no virtual register uses on the instruction being considered for remat), our actual implementation inconsistently supports non-trivial remat, and certain backends (AMDGPU and RISC-V mostly) lie about instructions being trivial to abuse that. We want to allow non-triial remat more broadly, but first we need to do some cleanup to make it understandable what's going on. These three call sites are ones which appear to actually want the trivial definition, and appear fairly low risk to change. p.s. I'm deliberately *not* updating any APIs in this change, I'm going to do that as a followup once it's clear which category each callsite fits in.
preames
requested review from
arsenm,
jmorse,
lukel97,
davemgreen,
topperc,
guy-david,
efriedma-quic and
wangpc-pp
|
@llvm/pr-subscribers-backend-webassembly Author: Philip Reames (preames) ChangesThis is a preparatory change for an upcoming reorganization of our rematerialization APIs. Despite the interface being documented as "trivial" (meaning no virtual register uses on the instruction being considered for remat), our actual implementation inconsistently supports non-trivial remat, and certain backends (AMDGPU and RISC-V mostly) lie about instructions being trivial to abuse that. We want to allow non-triial remat more broadly, but first we need to do some cleanup to make it understandable what's going on. These three call sites are ones which appear to actually want the trivial definition, and appear fairly low risk to change. p.s. I'm deliberately not updating any APIs in this change, I'm going to do that as a followup once it's clear which category each callsite fits in. Full diff: https://github.com/llvm/llvm-project/pull/160319.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 8efc6f124a55d..cf2bc499fe5d6 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2213,7 +2213,11 @@ findPrologueEndLoc(const MachineFunction *MF) {
-> std::optional<std::pair<const MachineInstr *, bool>> {
// Is this instruction trivial data shuffling or frame-setup?
bool isCopy = (TII.isCopyInstr(MI) ? true : false);
- bool isTrivRemat = TII.isTriviallyReMaterializable(MI);
+ bool isTrivRemat =
+ TII.isTriviallyReMaterializable(MI) &&
+ llvm::all_of(MI.all_uses(), [](const MachineOperand &MO) {
+ return MO.getReg().isVirtual();
+ });
bool isFrameSetup = MI.getFlag(MachineInstr::FrameSetup);
if (!isFrameSetup && MI.getDebugLoc()) {
diff --git a/llvm/lib/CodeGen/RegAllocScore.cpp b/llvm/lib/CodeGen/RegAllocScore.cpp
index b86647dbe0a48..ce1eea3519b71 100644
--- a/llvm/lib/CodeGen/RegAllocScore.cpp
+++ b/llvm/lib/CodeGen/RegAllocScore.cpp
@@ -79,8 +79,11 @@ llvm::calculateRegAllocScore(const MachineFunction &MF,
return MBFI.getBlockFreqRelativeToEntryBlock(&MBB);
},
[&](const MachineInstr &MI) {
- return MF.getSubtarget().getInstrInfo()->isTriviallyReMaterializable(
- MI);
+ auto *TTI = MF.getSubtarget().getInstrInfo();
+ return TTI->isTriviallyReMaterializable(MI) &&
+ llvm::all_of(MI.all_uses(), [](const MachineOperand &MO) {
+ return MO.getReg().isVirtual();
+ });
});
}
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index 08ca20b5eef6e..7591541779884 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -260,7 +260,10 @@ static void query(const MachineInstr &MI, bool &Read, bool &Write,
// Test whether Def is safe and profitable to rematerialize.
static bool shouldRematerialize(const MachineInstr &Def,
const WebAssemblyInstrInfo *TII) {
- return Def.isAsCheapAsAMove() && TII->isTriviallyReMaterializable(Def);
+ return Def.isAsCheapAsAMove() && TII->isTriviallyReMaterializable(Def) &&
+ llvm::all_of(Def.all_uses(), [](const MachineOperand &MO) {
+ return MO.getReg().isVirtual();
+ });
}
// Identify the definition for this register at this point. This is a
|
|
@llvm/pr-subscribers-debuginfo Author: Philip Reames (preames) ChangesThis is a preparatory change for an upcoming reorganization of our rematerialization APIs. Despite the interface being documented as "trivial" (meaning no virtual register uses on the instruction being considered for remat), our actual implementation inconsistently supports non-trivial remat, and certain backends (AMDGPU and RISC-V mostly) lie about instructions being trivial to abuse that. We want to allow non-triial remat more broadly, but first we need to do some cleanup to make it understandable what's going on. These three call sites are ones which appear to actually want the trivial definition, and appear fairly low risk to change. p.s. I'm deliberately not updating any APIs in this change, I'm going to do that as a followup once it's clear which category each callsite fits in. Full diff: https://github.com/llvm/llvm-project/pull/160319.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 8efc6f124a55d..cf2bc499fe5d6 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2213,7 +2213,11 @@ findPrologueEndLoc(const MachineFunction *MF) {
-> std::optional<std::pair<const MachineInstr *, bool>> {
// Is this instruction trivial data shuffling or frame-setup?
bool isCopy = (TII.isCopyInstr(MI) ? true : false);
- bool isTrivRemat = TII.isTriviallyReMaterializable(MI);
+ bool isTrivRemat =
+ TII.isTriviallyReMaterializable(MI) &&
+ llvm::all_of(MI.all_uses(), [](const MachineOperand &MO) {
+ return MO.getReg().isVirtual();
+ });
bool isFrameSetup = MI.getFlag(MachineInstr::FrameSetup);
if (!isFrameSetup && MI.getDebugLoc()) {
diff --git a/llvm/lib/CodeGen/RegAllocScore.cpp b/llvm/lib/CodeGen/RegAllocScore.cpp
index b86647dbe0a48..ce1eea3519b71 100644
--- a/llvm/lib/CodeGen/RegAllocScore.cpp
+++ b/llvm/lib/CodeGen/RegAllocScore.cpp
@@ -79,8 +79,11 @@ llvm::calculateRegAllocScore(const MachineFunction &MF,
return MBFI.getBlockFreqRelativeToEntryBlock(&MBB);
},
[&](const MachineInstr &MI) {
- return MF.getSubtarget().getInstrInfo()->isTriviallyReMaterializable(
- MI);
+ auto *TTI = MF.getSubtarget().getInstrInfo();
+ return TTI->isTriviallyReMaterializable(MI) &&
+ llvm::all_of(MI.all_uses(), [](const MachineOperand &MO) {
+ return MO.getReg().isVirtual();
+ });
});
}
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index 08ca20b5eef6e..7591541779884 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -260,7 +260,10 @@ static void query(const MachineInstr &MI, bool &Read, bool &Write,
// Test whether Def is safe and profitable to rematerialize.
static bool shouldRematerialize(const MachineInstr &Def,
const WebAssemblyInstrInfo *TII) {
- return Def.isAsCheapAsAMove() && TII->isTriviallyReMaterializable(Def);
+ return Def.isAsCheapAsAMove() && TII->isTriviallyReMaterializable(Def) &&
+ llvm::all_of(Def.all_uses(), [](const MachineOperand &MO) {
+ return MO.getReg().isVirtual();
+ });
}
// Identify the definition for this register at this point. This is a
|
|
I think this is safe for the portions changed in DwarfDebug.( If it's relying on the inconsistent behaviour you mention then we've got other validation processes that should pick it up). |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/26056 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/25901 Here is the relevant piece of the build log for the reference |
|
Noting I've seen the two failures reported by CI, and believe them to be real. This bot is testing an off by default configuration I can't reproduce locally, so I'm going to have to do a speculative fix. Running out in a moment for an errand, but will try to get these fixed a bit later today. |
preames
added a commit
to preames/llvm-project
that referenced
this pull request
Sep 23, 2025
This change builds on llvm#160319 which tries to clarify which *callers* (not backends) assume that the result is actually trivial. This change itself should be NFC. Essentially, I'm just renaming the existing isTrivialRematerializable to the non-trivial version and then adding a new trivial version (with the same name as the prior function) and simplifying a few callers which want that semantic. This change does *not* enable non-trivial remat any more broadly than was already done for our targets which were lying through the old APIs; that will come separately. The goal here is simply to make the code easier to follow in terms of what assumptions are being made where.
arsenm
reviewed
Sep 24, 2025
Comment on lines
+264
to
+266
| llvm::all_of(Def.all_uses(), [](const MachineOperand &MO) { | ||
| return MO.getReg().isVirtual(); | ||
| }); |
There was a problem hiding this comment.
Should put this in a utility function somewhere
There was a problem hiding this comment.
This particular one is folded away in #160377, but in general, yes.
There was a problem hiding this comment.
Actually, looking at this again, aren't I missing an isReg() check here? How is this not failing?
preames
added a commit
that referenced
this pull request
Sep 25, 2025
This change builds on #160319 which tries to clarify which *callers* (not backends) assume that the result is actually trivial. This change itself should be NFC. Essentially, I'm just renaming the existing isTrivialRematerializable to the non-trivial version and then adding a new trivial version (with the same name as the prior function) and simplifying a few callers which want that semantic. This change does *not* enable non-trivial remat any more broadly than was already done for our targets which were lying through the old APIs; that will come separately. The goal here is simply to make the code easier to follow in terms of what assumptions are being made where. --------- Co-authored-by: Luke Lau <luke_lau@icloud.com>
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 25, 2025
…fc] (#160377) This change builds on llvm/llvm-project#160319 which tries to clarify which *callers* (not backends) assume that the result is actually trivial. This change itself should be NFC. Essentially, I'm just renaming the existing isTrivialRematerializable to the non-trivial version and then adding a new trivial version (with the same name as the prior function) and simplifying a few callers which want that semantic. This change does *not* enable non-trivial remat any more broadly than was already done for our targets which were lying through the old APIs; that will come separately. The goal here is simply to make the code easier to follow in terms of what assumptions are being made where. --------- Co-authored-by: Luke Lau <luke_lau@icloud.com>
|
Warning to anyone looking at this later, the landed version contained a nasty bug where I'd accidentally reversed the condition being checked for. This was (accidentally) undone in #160377 but results in this change descriptions being super misleading. Sorry! |
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
…60377) This change builds on llvm#160319 which tries to clarify which *callers* (not backends) assume that the result is actually trivial. This change itself should be NFC. Essentially, I'm just renaming the existing isTrivialRematerializable to the non-trivial version and then adding a new trivial version (with the same name as the prior function) and simplifying a few callers which want that semantic. This change does *not* enable non-trivial remat any more broadly than was already done for our targets which were lying through the old APIs; that will come separately. The goal here is simply to make the code easier to follow in terms of what assumptions are being made where. --------- Co-authored-by: Luke Lau <luke_lau@icloud.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a preparatory change for an upcoming reorganization of our rematerialization APIs. Despite the interface being documented as "trivial" (meaning no virtual register uses on the instruction being considered for remat), our actual implementation inconsistently supports non-trivial remat, and certain backends (AMDGPU and RISC-V mostly) lie about instructions being trivial to abuse that. We want to allow non-triial remat more broadly, but first we need to do some cleanup to make it understandable what's going on.
These three call sites are ones which appear to actually want the trivial definition, and appear fairly low risk to change.
p.s. I'm deliberately not updating any APIs in this change, I'm going to do that as a followup once it's clear which category each callsite fits in.
Edit: Warning to anyone looking at this later, the landed version contained a nasty bug where I'd accidentally reversed the condition being checked for. This was (accidentally) undone in #160377 but results in this change description being super misleading. Sorry!